Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

auth: Use a custom function to determine access token expiry #362

Merged
merged 1 commit into from
Mar 10, 2023

Conversation

manicminer
Copy link
Contributor

@manicminer manicminer commented Mar 9, 2023

The default method *oauth2.Token{}.Valid() has a hardcoded delta of 10 seconds, which means that tokens are only renewed 10 seconds before they expire. This leads to race conditions during long running operations with the Resource Manager API, when a polling request is built immediately prior to this window, but only sent on or after the expiry time. Additionally, it's plausible that 1 (or more) second(s) may be lost during the process of token issuance, since it's likely the token is generated prior to being returned by the API, and it is the latter that informs us of the expiry time.

This change extends this window to 10 minutes for any access token with a validity period of 20+ minutes. For tokens having a validity period less than 20 minutes, those are renewed when 50% or more of that validity period has elapsed.

For example, a token that is valid for 1 hour (very common) will be renewed after it has been held for 50 minutes. However, a token issued for 10 minutes will be renewed when it has been held for 5 minutes (i.e. >=50% of the validity period).

Related: hashicorp/terraform-provider-azurerm#20834

@manicminer manicminer requested a review from a team as a code owner March 9, 2023 21:26
@manicminer manicminer force-pushed the bugfix/token-expiry-race-condition branch from 2f44408 to b0263ad Compare March 9, 2023 21:30
@manicminer manicminer changed the title Use a custom function to determine access token expiry auth: Use a custom function to determine access token expiry Mar 9, 2023
@manicminer manicminer force-pushed the bugfix/token-expiry-race-condition branch from b0263ad to d1a39ca Compare March 9, 2023 21:45
The default method `*oauth2.Token{}.Valid()` has a hardcoded delta of 10
seconds, which means that tokens are only renewed 10 seconds before they
expire. This leads to race conditions during long running operations
with the Resource Manager API, when a polling request is built
immediately prior to this window, but only sent on or after the expiry
time. Additionally, it's plausible that 1 (or more) second(s) may be
lost during the process of token issuance, since it's likely the token
is generated prior to being returned by the API, and it is the latter
that informs us of the expiry time.

This change extends this window to 10 minutes for any access token with
a validity period of 20+ minutes. For tokens having a validity period
less than 20 minutes, those are renewed when 50% or more of that
validity period has elapsed.

For example, a token that is valid for 1 hour (very common) will be
renewed after it has been held for 50 minutes. However, a token issued
for 10 minutes will be renewed when it has been held for 5 minutes (i.e.
>=50% of the validity period).
@manicminer manicminer force-pushed the bugfix/token-expiry-race-condition branch from d1a39ca to 86de601 Compare March 9, 2023 21:49
Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One comment about timezones but otherwise this LGTM 👍


expiry := token.Expiry.Round(0)
delta := tokenExpiryDelta
now := time.Now()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC the access token expiration time is in UTC, so do we want to explicitly convert these to UTC too?

Copy link
Contributor Author

@manicminer manicminer Mar 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's in local time actually, we're computing the expiration time based on the expires_in field from the login response, which is a seconds value that we add to the local time.

Screenshot 2023-03-10 at 10 25 41

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants